Perf and fixups #481

CodingDoug · 2016-12-29T23:34:51Z

Some optimizations - one in particular improves child change events to constant time from linear time (problematic for long lists). Also modernized and fixed up the database tests.
@samtstern @puf

samtstern · 2017-01-03T16:23:55Z

database/src/main/java/com/firebase/ui/database/FirebaseArray.java


 /**
 * This class implements an array-like collection on top of a Firebase location.
 */
 class FirebaseArray implements ChildEventListener {
-    public interface OnChangedListener {
+    interface OnChangedListener {


@CodingDoug why change the visibility on this interface as well as the constructor below? I believe @puf just got done approving this particular visibility.

Studio gives me a lint warning saying it's not necessary, which means it's not actually being used outside the package, just like the FirebaseArray class itself.

Yeah that's true but Studio is not really considering that this is a library for public consumption, so people may want to subclass etc.

How will anyone subclass OnChangedListener if it's inside a package private class? They won't be able to see it. Are we expecting that people put their classes inside this package so they can make use of it? Also, this interface isn't exposed anywhere in in the public API surface, so there would be no point in extending or implementing it.

If there is a use case here, maybe that needs some more elaboration?

samtstern · 2017-01-03T16:25:12Z

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

@@ -97,6 +97,7 @@ public void onChildMoved(DataSnapshot snapshot, String previousChildKey) {
        mSnapshots.remove(oldIndex);
        int newIndex = previousChildKey == null ? 0 : (getIndexForKey(previousChildKey) + 1);
        mSnapshots.add(newIndex, snapshot);
+        mSnapshotMap.put(snapshot.getKey(), newIndex);


Wouldn't onChildMoved affect the indexes of every key? I have a feeling that's why @puf didn't use a Map in the first place but I could be wrong.

Yup. This won't work.

All the tests passed on my end. Do we need more tests?

I don't think we have any tests that actually cover item re-order events, we should add one if we are going to change this functionality.

Note that the ordering implied by mSnapshots is not changed. All that's really changed here is what getIndexForKey() does to fulfill its job. There can be only one item with a particular key here, and that relationship is simply be expressed in the map now, so it doesn't have to scan the list for the unique key.

There is a test that checks for order changes called testChangePriorities(). It alters the priority of an item so that the array order changes from 1,2,3 to 3,1,2. This test passes.

samtstern · 2017-01-03T16:26:24Z

database/src/main/java/com/firebase/ui/database/FirebaseIndexArray.java

-        for (Query ref : refs) {
-            ref.removeEventListener(mRefs.remove(ref));
+        for (Map.Entry<Query, ValueEventListener> entry : mRefs.entrySet()) {
+            entry.getKey().removeEventListener(entry.getValue());


This is not equivalent to the previous behavior because there is no call to mRefs.remove()

samtstern · 2017-01-03T16:27:02Z

Added some comments but want @puf to be the reviewer here as it's his code.

puf · 2017-01-03T18:32:51Z

I highly recommend doing a performance test before proposing this change. The performance of looking up keys is indeed quadratic, but when we measured it a few months ago that only started becoming significant once we added > 1000 items, which is far beyond what I'd recommend doing.

SUPERCILEX · 2017-01-03T22:03:31Z

common/constants.gradle

@@ -5,6 +5,6 @@ project.ext.submodules = ['database', 'auth', 'storage']
 project.ext.group = 'com.firebaseui'
 project.ext.version = '1.0.1'
 project.ext.pomdesc = 'Firebase UI Android'
-project.ext.buildTools = '25.0.2'


@CodingDoug the version-1.1.0-dev branch has the latest everything (build tools, support lib, gradle build plugin, etc...). Changing the base branch of this PR to version-1.1.0-dev would minimize merge conflicts when merging it back into master! 😄

puf · 2017-01-03T22:10:13Z

I tested the reordering thoroughly at some point before we made the first public release. But that was based on having a script set random priorities on the items, which I never felt good enough to commit (and have long lost since).

…

On Tue, Jan 3, 2017 at 2:04 PM Sam Stern ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In database/src/main/java/com/firebase/ui/database/FirebaseArray.java <#481>: > @@ -97,6 +97,7 @@ public void onChildMoved(DataSnapshot snapshot, String previousChildKey) { mSnapshots.remove(oldIndex); int newIndex = previousChildKey == null ? 0 : (getIndexForKey(previousChildKey) + 1); mSnapshots.add(newIndex, snapshot); + mSnapshotMap.put(snapshot.getKey(), newIndex); I don't think we have any tests that actually cover item re-order events, we should add one if we are going to change this functionality. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#481>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3w31tWoQzW9DtcK1YagAhiik5QAtyAks5rOsXigaJpZM4LX6fN> .

samtstern · 2017-01-03T22:21:18Z

database/src/main/java/com/firebase/ui/database/FirebaseIndexArray.java

            entry.getKey().removeEventListener(entry.getValue());
+            it.remove();


Does calling Iterator#remove() on an Iterator created from .entrySet() actually delete from the underlying map? Maybe we should add a test that after cleanup() we verify mRefs is empty.

Yes, that case is specifically mentioned in the javadoc for Map.entrySet(). https://developer.android.com/reference/java/util/Map.html#entrySet()

CodingDoug · 2017-01-03T22:21:53Z

@puf The performance test in a simple situation may prove to be "OK", but the fact of the matter is that other work may end up being executed alongside this in the same "batch" of work on the main thread, and if that entire batch exceeds 16ms, Android starts dropping frames. This is a preventative measure to make sure this adapter doesn't work unnecessarily against that 16ms barrier.

Alternately, if we don't think FirebaseRecyclerAdapter is a good choice in some known situations, we should probably document those situations so people aren't mislead at the time a choice is being made.

CodingDoug · 2017-01-05T21:42:14Z

@samtstern Found that when an item moves front to back (rather than back to front, as was only being tested previously), there is an inconsistency with the recordkeeping of the new map. Fixed with a new test to verify that.

samtstern · 2017-01-05T22:41:34Z

@puf all my concerns are satisfied now, PTAL to see if yours are as well.

puf · 2017-01-08T16:32:23Z

Before adding code for performance improvements, let's have a look at what the (order of) performance of cases is in the current release and will be after this PR.

Append item (at end)
Insert item (anywhere)
Move item (anywhere)
Remove item (anywhere)
Read N items (sequential)
Read N random items

Up front: #1 is by far the most common write operation, so its performance should not decrease to benefit #2 and #3. I think #5 is the most common read patter, but that is more based on web knowledge. In fact, I wonder if scrolling up doesn't lead to a 5b where we get the items in reverse.

puf · 2017-01-08T17:56:27Z

I did some experiments with the performance of various write operations. My complete code is here: https://gist.github.com/puf/b2f2940188e11971afc688674d7a1cee

The current getIndexForKey() is an O(n^2) operation. If we make 2x more items to the list, looking up the index for a key takes 4x more time.

append is currently an O(log n) operation. It solely depends on ArrayList.add performance for append cases.
insert is currently an O(n^2) operation, because getIndexForKey() is O(n^2).
move is currently an O(n^2) operation. Strictly speaking 2*O(n^2), since we do two key lookups.
remove is currently an O(n^2) operation, since it relies on getIndexForKey().

puf · 2017-01-08T18:52:01Z

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

@@ -74,6 +83,7 @@ public void onChildAdded(DataSnapshot snapshot, String previousChildKey) {
            index = getIndexForKey(previousChildKey) + 1;


Shouldn't this case be rebuilding the "key-to-index" map too?

Perhaps! Could you write a test that should succeed, but instead fails?

puf · 2017-01-08T21:55:44Z

In the updated version getIndexForKey() is now an O(1) operation. The new scanArrayForKey() is an O(n^2) operation. Rebuilding the "key to index" map (in onChildMoved) is O(n) as far as I can tell (I didn't measure it yet).

append is O(log n) operation, but slightly slower now with an add to the end of an ArrayList and a Map.put().
insert is O(log n) operation, since it uses getIndexForKey()
move is O(n^2), since it depends on scanArrayForKey()
remove is O(1) now, since it uses getIndexForKey(). But it likely should be O(n), since it should rebuild the "key to index" map.

CodingDoug · 2017-01-09T00:51:10Z

@puf I believe scanArrayForKey is O(n) since it simply iterates a list at most once, looking for the matching key.

puf · 2017-01-09T01:16:19Z

When I tested getIndexForKey() (link) it turned out to be O(n^2). Hence I also tested scanArrayForKey() and (not surprisingly at this point anymore) it behaved as O(n^2) too. I updated the code in the gist if you feel like testing it or checking if I made a mistake in copying the logic.

CodingDoug · 2017-01-09T02:30:26Z

@puf Iteration of an array is definitely not quadratic. Your test harness is making measurements that are inherently quadratic in nature, based on the value of RUN_SIZE. It's performing RUN_SIZE number of lookups against an array of size RUN_SIZE, iterating through RUN_SIZE/2 elements each time. This can only cause quadratic gains as RUN_SIZE increases. If you limit the number of lookups to something constant instead of RUN_SIZE, and you cause some appropriate variance in the values you're looking for, you'll see linear gains at each step up in RUN_SIZE.

For example, instead of this:

for (int i=0; i < count; i++) {
    scanArrayForKey(list, i / 2);
}

You could say this to test the worst case scenario to be more fair to the value of NUM_SIZE:

for (int i=0; i < 10000; i++) {
    scanArrayForKey(list, (int) count-1);
}

Or, even more accurate to real life, you could use random numbers for the value to look up and assume that a large number of iterations will average things out.

puf · 2017-01-09T03:22:52Z

Ah... I get what you're saying: RUN_SIZE should only change the list size,
not the number of lookups. Good catch, I was already wondering why both
were quadratic.

For performing the append/insert I should loop over count items, but not
for timing the lookups. That definitely make a difference.

With that change, the lookups both getIndexForKey and scanArrayForKey
times grow sublinear with the increased number of items in the list. In
fact: they're almost at constant performance now.

samtstern · 2017-01-09T17:33:33Z

database/src/androidTest/java/com/firebase/ui/database/FirebaseArrayTest.java

@@ -129,4 +123,17 @@ public Boolean call() throws Exception {
            }
        });
    }
+
+    @Test
+    public void testChangePriorityFrontToBack() throws Exception {


samtstern · 2017-01-10T21:28:10Z

I am going to start doing QA on version 1.1.0 as there are many important fixes in there and it's been longer than expected. I have targeted this PR for 1.2.0 which will be the next minor release.

Moved TestUtils into correct directory for its package. Removed redundant tests. Reformatted some code and renamed methods for clarity.

…ant time perf during child changes instead of linear time. Removed interfaces internally where exact types would be faster.

SUPERCILEX · 2017-02-01T00:21:15Z

@samtstern I see you added this to "Needs decision". If we don't want to use HashMaps, I would really like to see the test fixups go in since we're currently using stuff that's deprecated and way out of date.

samtstern · 2017-02-09T16:38:05Z

#579 has been merged so this PR will likely want a rebase.

samtstern · 2017-04-26T15:18:04Z

Closing this since we haven't discussed this in ~months and it's not in a mergeable state anyway. I believe we've incorporated many of the testing changes. @CodingDoug if you ever convince @puf about all of this we can get back into it,

samtstern reviewed Jan 3, 2017

View reviewed changes

samtstern assigned puf Jan 3, 2017

SUPERCILEX reviewed Jan 3, 2017

View reviewed changes

samtstern reviewed Jan 3, 2017

View reviewed changes

samtstern approved these changes Jan 5, 2017

View reviewed changes

puf reviewed Jan 8, 2017

View reviewed changes

samtstern reviewed Jan 9, 2017

View reviewed changes

samtstern modified the milestone: 1.2.0 Jan 10, 2017

CodingDoug added 4 commits January 16, 2017 20:46

Modernized Android unit tests.

6babf71

Moved TestUtils into correct directory for its package. Removed redundant tests. Reformatted some code and renamed methods for clarity.

Fixed a performance issue with long lists of items - should see const…

e6cb1d8

…ant time perf during child changes instead of linear time. Removed interfaces internally where exact types would be faster.

Fix cleanup() without having to make a copy of mRefs.

c2a62e2

Fixed a bug and added another test that verifies it.

69dc670

CodingDoug force-pushed the perf-and-fixups branch from 0e04c76 to 69dc670 Compare January 17, 2017 04:57

This was referenced Jan 25, 2017

Add a callback in FirebaseIndexArray when a Key doesn't exist in a ref #448

Closed

The Big Kahuna of firebase-ui-database #544

Merged

SUPERCILEX mentioned this pull request Feb 7, 2017

Modernize database tests and cleanup #579

Merged

samtstern removed this from the 1.2.0 milestone Feb 21, 2017

samtstern closed this Apr 26, 2017

		entry.getKey().removeEventListener(entry.getValue());
		it.remove();

		@@ -74,6 +83,7 @@ public void onChildAdded(DataSnapshot snapshot, String previousChildKey) {
		index = getIndexForKey(previousChildKey) + 1;

Perf and fixups #481

Perf and fixups #481

Uh oh!

Conversation

CodingDoug commented Dec 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jan 3, 2017

Uh oh!

puf commented Jan 3, 2017

Uh oh!

SUPERCILEX Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puf commented Jan 3, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodingDoug commented Jan 3, 2017

Uh oh!

CodingDoug commented Jan 5, 2017

Uh oh!

samtstern commented Jan 5, 2017

Uh oh!

puf commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puf commented Jan 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puf commented Jan 8, 2017

Uh oh!

CodingDoug commented Jan 9, 2017

Uh oh!

puf commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodingDoug commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puf commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jan 10, 2017

Uh oh!

SUPERCILEX commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern commented Feb 9, 2017

Uh oh!

samtstern commented Apr 26, 2017

Uh oh!

SUPERCILEX Jan 3, 2017 •

edited

Loading

puf commented Jan 8, 2017 •

edited

Loading

puf commented Jan 9, 2017 •

edited

Loading

CodingDoug commented Jan 9, 2017 •

edited

Loading

SUPERCILEX commented Feb 1, 2017 •

edited

Loading